Set sequences & handle spurious wakeups in RVC4 IMU#1727
Conversation
📝 WalkthroughWalkthroughThis pull request updates a CMake configuration variable for RVC4 device versioning and adds a Catch2 test that validates IMU node sequence continuity by polling IMU packets from a running pipeline. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the on-device IMU test suite to better detect cases where IMU packets are emitted without any sensor report actually advancing (e.g., due to spurious wakeups), and bumps the tracked RVC4 device snapshot version.
Changes:
- Add a new IMU test that asserts at least one of the accelerometer/gyroscope sequence numbers advances across received packets.
- Configure IMU batching parameters for the new test case.
- Update the RVC4 device snapshot version string in CMake config.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/src/ondevice_tests/pipeline/node/imu_test.cpp |
Adds a new Catch2 test that validates IMU packet sequence advancement over time. |
cmake/Depthai/DepthaiDeviceRVC4Config.cmake |
Updates the pinned RVC4 snapshot version identifier. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dai::IMUPacket previousPacket; | ||
|
|
||
| while(pipeline.isRunning() && std::chrono::steady_clock::now() - start <= std::chrono::seconds(10)) { | ||
| auto imuData = imuQueue->get<dai::IMUData>(); |
| dai::IMUPacket previousPacket; | ||
|
|
||
| while(pipeline.isRunning() && std::chrono::steady_clock::now() - start <= std::chrono::seconds(10)) { | ||
| auto imuData = imuQueue->get<dai::IMUData>(); | ||
| if(imuData == nullptr) continue; | ||
|
|
||
| for(const auto& imuPacket : imuData->packets) { |
| for(const auto& imuPacket : imuData->packets) { | ||
| REQUIRE((imuPacket.acceleroMeter.sequence > previousPacket.acceleroMeter.sequence || imuPacket.gyroscope.sequence > previousPacket.gyroscope.sequence)); | ||
| previousPacket = imuPacket; | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/src/ondevice_tests/pipeline/node/imu_test.cpp`:
- Line 45: Rename the test case string in the TEST_CASE invocation to a more
descriptive name that reflects its intent (e.g., "Test IMU sequence numbers
strictly increase" or "Test IMU packets contain new data (no spurious
wakeups)"); locate the TEST_CASE(...) macro in imu_test.cpp (the TEST_CASE
symbol) and replace the current literal "At least one measurement should be
updated" with the chosen clearer description, and update any test
documentation/comments or related assertions if they reference the old name.
- Around line 45-73: The test currently can pass without receiving IMU data;
modify the test that uses imu, imuQueue, previousPacket and the while loop to
track whether any imu packets were observed (e.g., increment a counter or set a
boolean inside the for(const auto& imuPacket : imuData->packets) loop) and after
pipeline.stop() add a REQUIRE that asserts at least one packet was received
(e.g., REQUIRE(receivedCount > 0) or REQUIRE(receivedAny == true)) so the test
fails if no IMU data arrived.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4abe5c65-c2d2-4ba8-aa18-899c3571cfd7
📒 Files selected for processing (2)
cmake/Depthai/DepthaiDeviceRVC4Config.cmaketests/src/ondevice_tests/pipeline/node/imu_test.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Upload results
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-16T11:17:12.819Z
Learnt from: pheec
Repo: luxonis/depthai-core PR: 1715
File: include/depthai/pipeline/node/PointCloud.hpp:34-98
Timestamp: 2026-03-16T11:17:12.819Z
Learning: In `include/depthai/pipeline/node/PointCloud.hpp`, the nested `Impl` class inside `PointCloud` is intentionally public (not private/forward-declared) because the on-host unit tests in `tests/src/onhost_tests/point_cloud_test.cpp` directly access `Impl` methods such as `setIntrinsics`, `setExtrinsics`, `useCPU`, `useCPUMT`, `computePointCloudDense`, `applyTransformation`, and `filterValidPoints`. Do not suggest making `Impl` private or forward-declaring it.
Applied to files:
tests/src/ondevice_tests/pipeline/node/imu_test.cpp
🪛 Cppcheck (2.20.0)
tests/src/ondevice_tests/pipeline/node/imu_test.cpp
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
🔇 Additional comments (2)
cmake/Depthai/DepthaiDeviceRVC4Config.cmake (1)
6-6: LGTM! Firmware version updated for RVC4 device.The version update from
b96987e2...to40ac229a...aligns with the firmware changes mentioned in the PR title for handling IMU sequences and spurious wakeups.tests/src/ondevice_tests/pipeline/node/imu_test.cpp (1)
3-3: LGTM! Necessary include for time-bounded test loop.The
<chrono>header is correctly added to support the time-bounded polling logic in the new test case.
| basicIMUTest(480.0f, {dai::IMUSensor::GYROSCOPE_RAW}, 0.8f); // TODO(Morato) - debug why some devices need so much tolerance | ||
| } | ||
|
|
||
| TEST_CASE("At least one measurement should be updated") { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider a more descriptive test name.
The current name "At least one measurement should be updated" could be clearer. Consider renaming to something like "Test IMU sequence numbers strictly increase" or "Test IMU packets contain new data (no spurious wakeups)" to better reflect the test's purpose.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/ondevice_tests/pipeline/node/imu_test.cpp` at line 45, Rename the
test case string in the TEST_CASE invocation to a more descriptive name that
reflects its intent (e.g., "Test IMU sequence numbers strictly increase" or
"Test IMU packets contain new data (no spurious wakeups)"); locate the
TEST_CASE(...) macro in imu_test.cpp (the TEST_CASE symbol) and replace the
current literal "At least one measurement should be updated" with the chosen
clearer description, and update any test documentation/comments or related
assertions if they reference the old name.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/src/ondevice_tests/pipeline/node/imu_test.cpp`:
- Around line 51-52: The test sets imu->setMaxBatchReports(20) which exceeds the
documented maximum of 5; update the IMU setter and/or the test: in the IMU class
(function IMU::setMaxBatchReports in src/pipeline/node/IMU.cpp) add
validation/clamping to enforce the documented limit (e.g., clamp value to 5 or
return/error when >5) and update the unit test in imu_test.cpp to use an allowed
value (<=5) such as 5 so it matches include/depthai/properties/IMUProperties.hpp
documentation and avoids firmware violations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 351f4410-61e2-43c5-afb8-f30497693082
📒 Files selected for processing (1)
tests/src/ondevice_tests/pipeline/node/imu_test.cpp
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-03-16T11:17:12.819Z
Learnt from: pheec
Repo: luxonis/depthai-core PR: 1715
File: include/depthai/pipeline/node/PointCloud.hpp:34-98
Timestamp: 2026-03-16T11:17:12.819Z
Learning: In `include/depthai/pipeline/node/PointCloud.hpp`, the nested `Impl` class inside `PointCloud` is intentionally public (not private/forward-declared) because the on-host unit tests in `tests/src/onhost_tests/point_cloud_test.cpp` directly access `Impl` methods such as `setIntrinsics`, `setExtrinsics`, `useCPU`, `useCPUMT`, `computePointCloudDense`, `applyTransformation`, and `filterValidPoints`. Do not suggest making `Impl` private or forward-declaring it.
Applied to files:
tests/src/ondevice_tests/pipeline/node/imu_test.cpp
📚 Learning: 2026-03-24T22:39:05.647Z
Learnt from: MaticTonin
Repo: luxonis/depthai-core PR: 1732
File: src/pipeline/Pipeline.cpp:705-705
Timestamp: 2026-03-24T22:39:05.647Z
Learning: In `src/pipeline/Pipeline.cpp`, within `PipelineImpl::build()`, the condition `autoCalibrationString != "OFF" && autoCalibrationString != ""` intentionally retains the empty-string check. Even though `utility::getEnvAs<std::string>("DEPTHAI_AUTOCALIBRATION", "ON_START")` provides a default of `"ON_START"`, the explicit `!= ""` guard is kept on purpose and should not be flagged as redundant in future reviews.
Applied to files:
tests/src/ondevice_tests/pipeline/node/imu_test.cpp
🪛 Cppcheck (2.20.0)
tests/src/ondevice_tests/pipeline/node/imu_test.cpp
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
🔇 Additional comments (2)
tests/src/ondevice_tests/pipeline/node/imu_test.cpp (2)
3-3: LGTM!The
<chrono>include is correctly added to supportstd::chrono::steady_clock::now()andstd::chrono::seconds()used in the new test.
60-73: Test logic for sequence validation is sound.The use of an OR condition (
||) in the sequence check at line 71 is appropriate. Per theIMUPacketdocumentation: "Only the enabled outputs are populated," so each sensor may advance independently. The assertion correctly verifies that at least one sensor's sequence has increased, which detects spurious wakeups (where a wakeup occurs but no new data is available).The
numMessages > 0assertion at line 76 properly addresses the concern about the test passing trivially if no data is received.
| imu->setBatchReportThreshold(10); | ||
| imu->setMaxBatchReports(20); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify maxBatchReports constraint documentation and runtime handling
# Check the IMUProperties documentation
echo "=== IMUProperties maxBatchReports documentation ==="
rg -n -A2 -B2 'maxBatchReports' --type=cpp
# Check if there's any validation/clamping logic for maxBatchReports
echo ""
echo "=== Searching for validation of maxBatchReports ==="
rg -n -C5 'setMaxBatchReports' --type=cppRepository: luxonis/depthai-core
Length of output: 12251
setMaxBatchReports(20) exceeds the documented maximum of 5.
According to include/depthai/properties/IMUProperties.hpp:172-173, maxBatchReports is documented as "Maximum 5," yet the test sets this to 20. The setter in src/pipeline/node/IMU.cpp:42-43 does not validate or clamp this value, so undefined behavior or firmware-level constraint violations are possible.
🔧 Proposed fix
imu->setBatchReportThreshold(10);
- imu->setMaxBatchReports(20);
+ imu->setMaxBatchReports(5);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| imu->setBatchReportThreshold(10); | |
| imu->setMaxBatchReports(20); | |
| imu->setBatchReportThreshold(10); | |
| imu->setMaxBatchReports(5); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/ondevice_tests/pipeline/node/imu_test.cpp` around lines 51 - 52,
The test sets imu->setMaxBatchReports(20) which exceeds the documented maximum
of 5; update the IMU setter and/or the test: in the IMU class (function
IMU::setMaxBatchReports in src/pipeline/node/IMU.cpp) add validation/clamping to
enforce the documented limit (e.g., clamp value to 5 or return/error when >5)
and update the unit test in imu_test.cpp to use an allowed value (<=5) such as 5
so it matches include/depthai/properties/IMUProperties.hpp documentation and
avoids firmware violations.
asahtik
added
testable
Purpose
Specification
None / not applicable
Dependencies & Potential Impact
None / not applicable
Deployment Plan
None / not applicable
Testing & Validation
None / not applicable
Summary by CodeRabbit
Chores
Tests